Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix misplacement of geometry parts #30

Merged
merged 23 commits into from
Aug 14, 2024
Merged

Fix misplacement of geometry parts #30

merged 23 commits into from
Aug 14, 2024

Conversation

FObermaier
Copy link
Member

If components of geometries are too small to be encoded at the provided zoom level the encoded data is deleted in Encode(CoordinateSequence seq, ..., ref int currentX, ref int currentY, ...) but the values of currentX and currentY are not reset to their initial values. This causes all subsequent geometry items to have a wrong offset.

This PR fixes this by

  • storing initial currentX and currentY values, and reseting their values when encoded data is deleted.
  • checking for all geometry components individually if their extent exceeds 1 pixel in the tile at the given zoom level.

closes #29

@FObermaier FObermaier requested a review from xivk March 19, 2024 10:15
@FObermaier
Copy link
Member Author

@rbrundritt, @xivk, @kolyanch
I'd like your opinion on whether or not we should check if the extent of an encoded LineString exceeds 1 pixel and only write it to the tile if it does:

private static IEnumerable<uint> Encode(ILineal lineal, TileGeometryTransform tgt)
{
var geometry = (Geometry)lineal;
int currentX = 0, currentY = 0;
for (int i = 0; i < geometry.NumGeometries; i++)
{
var lineString = (LineString)geometry.GetGeometryN(i);
if (tgt.IsGreaterThanOnePixelOfTile(lineString))
{
foreach (uint encoded in Encode(lineString.CoordinateSequence, tgt, ref currentX, ref currentY, false))
yield return encoded;
}
}
}

@kolyanch
Copy link
Contributor

I believe that the processing of small objects should be the same for polygonal and linear objects. I opened the MVT specification, there is nothing about linear objects. But there is a strict rule for polygonal ones:

A linear ring SHOULD NOT have an area calculated by the surveyor's formula equal to zero, as this would signify a ring with anomalous geometric points

In short I vote for checks for linear objects)

@rbrundritt
Copy link
Contributor

I did think about this, as well as the 1-pixel area polygon scenario (or user defined minimums). The main issue I've ran into in the past with this is you end up in scenarios where the end user is zoomed out and you have an empty map and can't see where the data is. I've found that it is often better to show 1-pixel for lines/polygons in this scenario, at least from a real-world user experience perspective. That said, it would depend on your data set. If you are showing a single data set, it's nice to have these show, but if you have multiple datasets where shapes are different sizes, then it may be desirable to not have the smaller shapes appear.

With all this in mind, having a configuration setting on a per layer level, where the minimum area/length of a polygon/line could be set, would be a "nice to have" feature. But this definitely shouldn't be forced.

@xivk
Copy link
Contributor

xivk commented Mar 20, 2024

I also believe by default we should drop anything that is too small. I even thought this was the default behavior, I have some checking to do on some of our services now...

@FObermaier
Copy link
Member Author

I went with @rbrundritt suggestion to have it configurable. MapBoxTileWriter has Write overloads that take minLinealExtent (default: 1) and minPolygonalExtent (default: 2, resembles previous greater than 1 constraint).
The implementation is a bit akward as I did not want to break binary compatibility.

Please reeavaluate @rbrundritt , @xivk and @kolyanch.

@FObermaier FObermaier merged commit 437d29e into develop Aug 14, 2024
4 checks passed
@FObermaier FObermaier deleted the fix/issue29 branch August 14, 2024 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MVT Writer Geometry shift
5 participants